-
Notifications
You must be signed in to change notification settings - Fork 25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issue 357: Nodejs ReaderGroup and EventReader #358
Issue 357: Nodejs ReaderGroup and EventReader #358
Conversation
Signed-off-by: thekingofcity <3353040+thekingofcity@users.noreply.github.com>
Signed-off-by: thekingofcity <3353040+thekingofcity@users.noreply.github.com>
Signed-off-by: thekingofcity <3353040+thekingofcity@users.noreply.github.com>
Signed-off-by: thekingofcity <3353040+thekingofcity@users.noreply.github.com>
Signed-off-by: thekingofcity <3353040+thekingofcity@users.noreply.github.com>
Signed-off-by: thekingofcity <3353040+thekingofcity@users.noreply.github.com>
a12749f
to
c92c04c
Compare
/// | ||
/// Return the event offset in the segment. | ||
/// | ||
pub fn js_offset(mut cx: FunctionContext) -> JsResult<JsNumber> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to expose this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure. Will exposing this cause confusion to users? The Python __repr__
implementation of EventData
does expose the offset, so I'm assuming it's something that is ok to be exposed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's fine to pass it to the application and for them to pass it back to us via APIs. But we want to be clear that one cann't do pointer math with it. IE: It is not ok to generate your own integer based on this value and then use that to refer to something, because we don't guarantee that we won't have any header or compression or something in the future. So it's fine so long as the value is not interpreted.
In Java we do this by having an Object which wraps it which is opaque to users, so while it is just a long they can't directly get that long for visibility reasons. This is the normal pattern in Java to lock things down. In Python that is not the normal pattern and underscores are used in the name to indicate caution. I don't know what the norm is in JS. Is there a common pattern around this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a keyword called readonly
that works similarly to Python's underscore pattern, except you can not change the variable. But if you've written Python or JavaScript or any similar dynamic code, you would know users could access and manipulate anything they want. So instead of posing some restrictions on offset, it may be better to state it in doc or comment and let users be clear about what they are doing.
Besides, both py and js bindings don't allow users to specify a streamcut/offset on reading, so we may also improve this later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clippy has some comments also
Signed-off-by: thekingofcity <3353040+thekingofcity@users.noreply.github.com>
Signed-off-by: thekingofcity <3353040+thekingofcity@users.noreply.github.com>
Signed-off-by: thekingofcity <3353040+thekingofcity@users.noreply.github.com>
CI tests and clippy problems are fixed. |
Signed-off-by: thekingofcity <3353040+thekingofcity@users.noreply.github.com>
Signed-off-by: thekingofcity <3353040+thekingofcity@users.noreply.github.com>
Signed-off-by: thekingofcity <3353040+thekingofcity@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved with comments
nodejs/src/stream_reader.rs
Outdated
/// | ||
/// Return a Slice in an asynchronous call. | ||
/// The actural returned type from await will be `Promise<Slice>` aka `JsResult<JsBoxed<RefCell<Slice>>>`. | ||
/// | ||
/// See the tokio-fetch example for more details on how to return a Promise and await. | ||
/// https://github.com/neon-bindings/examples/tree/2dbbef55f483635d0118c20c9902bf4c6faa1ecc/examples/tokio-fetch | ||
/// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this meant to be a user facing comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, all comments in rust should not be a user facing comment/doc. Users are advised to read the README.md for a tutorial and https://pravega.github.io/pravega-client-rust/nodejs/index.html for a full API reference(auto generated from ts files and contains user facing comments.) The API reference is added in https://github.com/pravega/pravega-client-rust/pull/366/files#diff-d7ecc9db1d2f1f75440ba2c3e433aec8c252e9c86a6260732400737857c71d3b and should be available in the github.io pages after merge.
Signed-off-by: thekingofcity <3353040+thekingofcity@users.noreply.github.com>
Signed-off-by: thekingofcity 3353040+thekingofcity@users.noreply.github.com
Change log description
Add
ReaderGroup
andEventReader
support to Nodejs bindings.Purpose of the change
Fixes #357
What the code does
Implement the
stream_reader_group
andstream_reader
just like the Python code.It also uses the asynchronous approach so that users can do the followings:
This should be merged after #355.
Reference about
@@iterator
:How to verify it
Since the writer is not here, I use Python code to create the stream and write something to Pravega.
npm run build-debug
python3 tests/write.py
will write 4 events.node --loader ts-node/esm tests/stream_reader.ts
should print these.I will write similar tests like Python after the writer is implemented.